Skip to content

Conversation

felixbarny
Copy link
Member

This following was problematic:

.put("index.routing_path", randomBoolean() ? null : "metricset")

Because it set an explicit null as the routing path`.

In cases where the following evaluates to false, this leads to an error rightfully complaining that the routing path is required:

.put("index.dimensions_tsid_strategy_enabled", randomDouble() < 0.8)

@felixbarny felixbarny self-assigned this Oct 6, 2025
@felixbarny felixbarny added >test Issues or PRs that are addressing/adding tests :StorageEngine/TSDB You know, for Metrics labels Oct 6, 2025
@felixbarny felixbarny linked an issue Oct 6, 2025 that may be closed by this pull request
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-storage-engine (Team:StorageEngine)

@elasticsearchmachine elasticsearchmachine added Team:StorageEngine v9.3.0 external-contributor Pull request authored by a developer outside the Elasticsearch team labels Oct 6, 2025
@felixbarny felixbarny enabled auto-merge (squash) October 6, 2025 11:46
Settings.Builder settingsBuilder = Settings.builder()
.put("index.mode", "time_series")
.put("index.dimensions_tsid_strategy_enabled", randomDouble() < 0.8);
if (randomBoolean()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this also be always set when the setting above is false?

Copy link
Contributor

@kkrik-es kkrik-es Oct 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd simply fix as:

boolean dimensions_tsid_strategy_enabled = randomDouble() < 0.8;
...
          .put("index.routing_path", dimensions_tsid_strategy_enabled || randomBoolean() ? "metricset" : null)
          .put("index.dimensions_tsid_strategy_enabled", dimensions_tsid_strategy_enabled)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this also be always set when the setting above is false?

No, even if index.dimensions_tsid_strategy_enabled is false, the routing_path doesn't need to be set explicitly as this is a data stream template, for which the routing_path will be automatically determined if unset. I explicitly want to test all four permutations of strategy enabled true/false and routing path set/unset. Also, routing path unset is different to routing path explicitly set to null. This triggered the test failure in the first place. It's a bit surprising but not sure if it's worth changing. At least not in this PR.

As both explicitly setting index.routing_path as well setting index.dimensions_tsid_strategy_enable to false disables the new index.dimensions-based tsid strategy, I wanted to give disabling that strategy a lower probability with via randomDouble() < 0.8.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that I also ran this locally with 500 iterations and all have succeeded.

@felixbarny felixbarny disabled auto-merge October 6, 2025 12:19
@felixbarny felixbarny merged commit cefc2ac into elastic:main Oct 6, 2025
34 checks passed
@felixbarny felixbarny deleted the fix-tsdb-it branch October 6, 2025 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external-contributor Pull request authored by a developer outside the Elasticsearch team :StorageEngine/TSDB You know, for Metrics Team:StorageEngine >test Issues or PRs that are addressing/adding tests v9.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CI] TSDBIndexingIT testTsdbTemplatesNoKeywordFieldType failing
3 participants